Repository navigation
fix(resolver): skip unreadable cwd ancestors - #31198
Closed
chocolatecake777 wants to merge 2 commits into
Closed
chocolatecake777 wants to merge 2 commits into
chocolatecake777 wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR adds graceful handling of permission-denied errors during module resolution directory traversal. When intermediate ancestor directories are inaccessible, the resolver now skips them and continues walking rather than aborting, allowing ChangesGraceful directory read error handling during module resolution
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/run_command.test.ts`:
- Around line 190-213: canUseLandlock() currently only verifies the ruleset
blocks / while allowing allowedDir but doesn't confirm that a real bun process
can start under that restricted allowlist; update the probe so the helper
(written by writeLandlockHelper) actually launches a trivial bun invocation
(e.g., bun --version or a minimal bun script) inside the landlocked environment
and the Bun.spawnSync call checks both LANDLOCK_OK and the bun invocation's
expected output; adjust the helper invocation arguments (helperPath and the
bunExe passed to it) and the spawn/result checks in canUseLandlock() so the
probe only returns true when the helper reports LANDLOCK_OK and the launched bun
command succeeds.
In `@test/js/bun/resolve/resolve.test.ts`:
- Around line 836-862: The test currently only asserts non-empty stderr and
non-zero exit, which can mask unrelated failures; update the assertion after
spawning Bun (variables proc, stdout, stderr, exitCode) to explicitly check that
stderr contains the unreadable-cwd error string (e.g.,
"CouldntReadCurrentDirectory" or the exact platform-specific message your
runtime emits) in addition to keeping the exitCode non-zero and stdout empty so
the test specifically verifies the unreadable-current-directory failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4c8c382-85b5-469d-a8c4-d86db75c3c13
📒 Files selected for processing (4)
src/jsc/bindings/wtf-bindings.cppsrc/resolver/resolver.rstest/cli/run/run_command.test.tstest/js/bun/resolve/resolve.test.ts
Comment on lines
+190
to
+213
| function canUseLandlock() { | ||
| if (!isLinux) return false; | ||
|
|
||
| const root = tempDirWithFiles("run-landlock-probe", {}); | ||
| try { | ||
| const testBase = join(root, "parent", "project"); | ||
| const helperPath = writeLandlockHelper(root); | ||
|
|
||
| mkdirSync(testBase, { recursive: true }); | ||
| const check = Bun.spawnSync({ | ||
| cmd: [bunExe(), helperPath, testBase, dirname(bunExe()), "--self-check"], | ||
| env: bunEnv, | ||
| cwd: testBase, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| return check.exitCode === 0 && check.stdout.toString().includes("LANDLOCK_OK"); | ||
| } finally { | ||
| try { | ||
| rmSync(root, { recursive: true, force: true }); | ||
| } catch {} | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make the Landlock probe validate a real bun launch.
canUseLandlock() only checks that the ruleset can block / while still opening allowedDir. It never proves that the restricted allowlist is sufficient to execute bun, so this can return true and enable the suite even when the sandbox later fails for unrelated loader/libc path reasons. Probe with a trivial bun invocation under the helper so the skip condition matches the actual test preconditions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/cli/run/run_command.test.ts` around lines 190 - 213, canUseLandlock()
currently only verifies the ruleset blocks / while allowing allowedDir but
doesn't confirm that a real bun process can start under that restricted
allowlist; update the probe so the helper (written by writeLandlockHelper)
actually launches a trivial bun invocation (e.g., bun --version or a minimal bun
script) inside the landlocked environment and the Bun.spawnSync call checks both
LANDLOCK_OK and the bun invocation's expected output; adjust the helper
invocation arguments (helperPath and the bunExe passed to it) and the
spawn/result checks in canUseLandlock() so the probe only returns true when the
helper reports LANDLOCK_OK and the launched bun command succeeds.
Comment on lines
+836
to
+862
| it.skipIf(!canTriggerEACCES)("module resolution still fails when cwd itself is unreadable", async () => { | ||
| using dir = tempDir("resolver-inaccessible-target", { | ||
| "project/entry.js": `console.log("should not run");\n`, | ||
| }); | ||
| const root = String(dir); | ||
| const project = join(root, "project"); | ||
|
|
||
| if (canUseRunuser) { | ||
| chmodSync(root, 0o755); | ||
| chmodSync(project, 0o755); | ||
| chmodSync(join(project, "entry.js"), 0o644); | ||
| } | ||
|
|
||
| try { | ||
| chmodSync(project, 0o111); | ||
| await using proc = Bun.spawn({ | ||
| cmd: canUseRunuser ? ["runuser", "-u", "nobody", "--", bunExe(), "entry.js"] : [bunExe(), "entry.js"], | ||
| env: bunEnv, | ||
| cwd: project, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe(""); | ||
| expect(stderr).not.toBe(""); | ||
| expect(exitCode).not.toBe(0); |
Contributor
There was a problem hiding this comment.
Assert the unreadable-cwd error explicitly.
This currently passes on any nonzero failure with any stderr, so the regression can stay green even if the process dies for an unrelated reason before hitting the target-directory path. Match the concrete error for this case (for example CouldntReadCurrentDirectory) so the test actually guards the behavior this PR is preserving.
Suggested tightening
- expect(stderr).not.toBe("");
+ expect(stderr).toContain("CouldntReadCurrentDirectory");
expect(exitCode).not.toBe(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.skipIf(!canTriggerEACCES)("module resolution still fails when cwd itself is unreadable", async () => { | |
| using dir = tempDir("resolver-inaccessible-target", { | |
| "project/entry.js": `console.log("should not run");\n`, | |
| }); | |
| const root = String(dir); | |
| const project = join(root, "project"); | |
| if (canUseRunuser) { | |
| chmodSync(root, 0o755); | |
| chmodSync(project, 0o755); | |
| chmodSync(join(project, "entry.js"), 0o644); | |
| } | |
| try { | |
| chmodSync(project, 0o111); | |
| await using proc = Bun.spawn({ | |
| cmd: canUseRunuser ? ["runuser", "-u", "nobody", "--", bunExe(), "entry.js"] : [bunExe(), "entry.js"], | |
| env: bunEnv, | |
| cwd: project, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stdout).toBe(""); | |
| expect(stderr).not.toBe(""); | |
| expect(exitCode).not.toBe(0); | |
| it.skipIf(!canTriggerEACCES)("module resolution still fails when cwd itself is unreadable", async () => { | |
| using dir = tempDir("resolver-inaccessible-target", { | |
| "project/entry.js": `console.log("should not run");\n`, | |
| }); | |
| const root = String(dir); | |
| const project = join(root, "project"); | |
| if (canUseRunuser) { | |
| chmodSync(root, 0o755); | |
| chmodSync(project, 0o755); | |
| chmodSync(join(project, "entry.js"), 0o644); | |
| } | |
| try { | |
| chmodSync(project, 0o111); | |
| await using proc = Bun.spawn({ | |
| cmd: canUseRunuser ? ["runuser", "-u", "nobody", "--", bunExe(), "entry.js"] : [bunExe(), "entry.js"], | |
| env: bunEnv, | |
| cwd: project, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stdout).toBe(""); | |
| expect(stderr).toContain("CouldntReadCurrentDirectory"); | |
| expect(exitCode).not.toBe(0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/resolve/resolve.test.ts` around lines 836 - 862, The test
currently only asserts non-empty stderr and non-zero exit, which can mask
unrelated failures; update the assertion after spawning Bun (variables proc,
stdout, stderr, exitCode) to explicitly check that stderr contains the
unreadable-cwd error string (e.g., "CouldntReadCurrentDirectory" or the exact
platform-specific message your runtime emits) in addition to keeping the
exitCode non-zero and stdout empty so the test specifically verifies the
unreadable-current-directory failure path.
|
Also fixes #28220 I think |
|
No updates? |
Leonisaurov
added a commit
to Leonisaurov/opencode-termux
that referenced
this pull request
Jul 27, 2026
…/Termux - Save PR #31198 diff (oven-sh/bun#31198) for reference - Add resolver.rs and wtf-bindings.cpp hunks to android-support.patch - Restore build-bun.yml to cross-compilation mode targeting Bun 1.3.14
Collaborator
|
Thank you for this contribution! This was fixed in #31938 and #33119: the resolver now treats EPERM/EACCES on ancestor directories as an empty dir and continues. The relevant code is in Closing as already fixed. We really appreciate you taking the time to track this down and submit a patch! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bun runtests that cover unreadable-ancestor behavior on Linux, plus the small debug-build include fix needed for validationIssue
Fixes #30859
What changed
<assert.h>insrc/jsc/bindings/wtf-bindings.cppso the patched debug binary builds cleanly for the validation path used hereVerification
docker_resolver_unreadable_ancestor_tests: passed (3 passed,1 skipped) with the patched debug binarydocker_landlock_bun_run_tests: passed (2 passed) with the patched debug binarydocker_prettier_modified_tests_check: passedReviewer Notes
Notes
Prepared with AI assistance and manually reviewed/tested.